Skip to content

feat(wallet): add BRC-98 ECPM semantic module - #488

Open
connormurray2 wants to merge 13 commits into
bsv-blockchain:mainfrom
connormurray2:brc-229-invert
Open

feat(wallet): add BRC-98 ECPM semantic module#488
connormurray2 wants to merge 13 commits into
bsv-blockchain:mainfrom
connormurray2:brc-229-invert

Conversation

@connormurray2

@connormurray2 connormurray2 commented Aug 20, 2026

Copy link
Copy Markdown

Summary

Implements BRC-229 as the installable @bsv/ecpm-permission-module BRC-98 semantic module. It replaces the earlier optional WalletInterface.multiplyPoint direction and preserves the fixed BRC-100 interface and Wallet Wire.

The companion specification is bsv-blockchain/BRCs#230.

Protocol

Applications continue to call getPublicKey with:

p ecpm <apply|remove> <pointHex> <logicalProtocolID>

The module derives one scalar under [securityLevel, "p ecpm <logicalProtocolID>"] plus the ordinary key ID and counterparty fields. The operation and point are deliberately excluded from the derivation identity:

  • apply returns d·P;
  • remove returns d⁻¹·P;
  • applying and removing therefore select the same scalar and round-trip exactly.

Logical protocol IDs remain 5–273 ASCII bytes. The complete BRC-98 dispatch envelope is explicitly bounded at 353 bytes for apply and 354 for remove, preserving the full BRC-43 logical namespace while remaining within BRC-100's 400-byte transport limit.

Integration and security

  • Adds an optional, backwards-compatible PermissionsModule.handleRequest semantic hook.
  • Registers ECPM under the ecpm module scheme and returns the existing { publicKey } result shape without invoking ordinary getPublicKey.
  • Rejects every non-getPublicKey operation, identity-key requests, and forSelf: true under p ecpm.
  • Validates canonical lowercase compressed secp256k1 points, including x-coordinate range checks before parsing.
  • Isolates ordinary and privileged derivation providers and scopes cached and concurrent privileged approvals to the exact privilegedReason.
  • Keeps all key derivers, root material, and private scalars inside trusted wallet/module code.

Verification

Exact head: 7751b42a48cd71bcb80cfaee1df9433c8250d823

  • ECPM unit/property coverage: 44 tests; 98.57% statements, 97.41% branches, 94.44% functions, 98.47% lines.
  • Governed ECPM mutation target: 86.67% (260 killed, 40 survived, 0 no-coverage, 0 invalid).
  • Package typecheck, lint, formatting, build, packed ESM consumer/declaration checks, property tests, and browser contract pass locally.
  • Repository health, workspace build/typecheck/lint/policy, documentation, conformance, coverage, browser/mobile platform contracts, dependency/security analysis, CodeQL, and zero-new-findings Sonar checks all pass in hosted CI.
  • Exact-head merge gate passed with 35 successful checks and 3 intentional scope-based skips.

All review conversations are resolved and the implementation is ready for final maintainer review.

…-side

Supersedes bsv-blockchain#487, which was overbuilt. That branch added two methods, a wire call
code and substrate plumbing across six transports. This is the single operation
actually missing, and nothing else.

Context. Masking a point through a BRC-100 wallet is already possible today, via
revealCounterpartyKeyLinkage plus decrypt, and masks produced that way commute
across independent wallets. What has no route through the interface is removing
a mask: that needs multiplication by the modular inverse of the derived key.
Feeding a*C back through the linkage recipe yields a^2*C, not C.

The primitives already exist and this method composes them rather than
introducing anything new:

  const masked   = new PublicKey(key.deriveSharedSecret(point))
  const inverse  = new PrivateKey(key.invm(new Curve().n))
  const unmasked = inverse.deriveSharedSecret(masked)   // === point

That composition requires the private key in application memory. WalletInterface
exposes 29 methods and no route to a scalar -- keyDeriver is a property of the
in-process class, not part of the interface, so over a substrate there is none.
An application whose keys live in a wallet therefore cannot complete the second
step. This method runs both steps where the key already is. The first test
asserts the output is identical to the composition above, so the behaviour is
pinned to the existing primitives rather than to a new definition.

Optional, deliberately. BRC-100's value is that it does not change, so a method
added later cannot be mandatory: declaring it required on WalletInterface broke
23 call sites across every substrate plus the KV store, registry and identity
clients, and declaring it required on ProtoWallet broke @bsv/wallet-toolbox,
where Wallet, PrivilegedKeyManager and the wallet managers satisfy the class
structurally without extending it. Applications feature-detect and degrade. Wire
substrate support is deliberately excluded here; it needs a call code, which is
an interface-version decision rather than a library one.

Key derivation is mandatory rather than stylistic. For a counterparty point Q,
d*Q IS the ECDH shared secret with Q, so performing this with a spending or
identity key would hand any caller that secret and break encryption to that
counterparty. The key is always derived from protocolID/keyID/counterparty and
no identityKey option is offered.

On validation: PublicKey.fromString accepts '02' + 'ff'.repeat(32), an
x-coordinate greater than the field prime, reduces it silently to 0x1000003d0,
and validate() then returns true. A test asserts both halves of that so the
reason for the range check is visible. The check runs before the parser, since
the parser performs the reduction. go-sdk has the same behaviour independently.

Verified: tsc -b clean, oxlint --deny-warnings clean, prettier clean on the
files this adds to (the two existing warnings in Wallet.interfaces.ts predate
it), full sdk suite green at 157 suites / 5925 tests, and wallet-toolbox at its
4 pre-existing TS2307 baseline with 0 attributable here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Connor Murray and others added 3 commits August 20, 2026 10:29
CI failure on bsv-blockchain#488 was six lanes -- merge-gate, build-and-test, browser
packages, SDK coverage, wallet browser and wallet mobile -- all one cause: bundle
size budgets, and zero type errors anywhere.

Measured rather than guessed. Building the UMD bundle from main's SDK gives
554475 bytes against a 555000 budget, so the ratchet was sitting 525 bytes above
head. multiplyPoint adds 963, which crosses it. These budgets are deliberate
ratchets set just above current size, so any real addition trips them and the
fix is to advance them by what the addition actually costs.

Reduced the cost before raising anything: the error messages carried a redundant
'the supplied'/'the result is' phrasing that bought no diagnostic value, since
the stack already names the function. Trimming those took the delta from 1043 to
963 bytes. Tests still pass -- they match on the specific part of each message,
not the prose.

Six raw budgets advanced to the next 1000-byte boundary above the observed size,
matching the existing convention:

  sdk umd          555000 -> 556000  (observed 555438)
  sdk esbuild      560000 -> 561000  (observed 560710)
  sdk vite         742000 -> 743000  (observed 742268)
  message-box umd  510000 -> 511000  (observed 510105)
  wallet client    1607000 -> 1608000 (observed 1607943)
  wallet mobile    3367000 -> 3368000 (observed 3367997)

Only the raw dimension moves. The checker throws on the first dimension over
budget, which would have meant discovering these one CI round at a time, so I
instrumented it locally to print every measurement at once and then restored it
unmodified. That surfaced the esbuild and vite overages before CI reported them.
Compressed dimensions have far more slack -- gzip sits 2808 under and brotli
4044 under, against 562 for raw -- because a kilobyte of new source compresses to
a few hundred bytes. CI agrees: every failing lane named raw and nothing else.

Verified: sdk test:browser passes the full exact-tarball browser contract, tsc -b
clean, oxlint clean repo-wide, prettier clean, and the multiplyPoint suite green
at 10 tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mension

Second round on the same cause, so this time it covers the whole surface rather
than only what CI named.

CI reported two more overages after the first budget commit: esbuild browser raw
at 1253340 against 1252500, and Hermes mobile bytecode gzip at 1366133 against
1366000. Zero type errors, again -- these are purely size ratchets.

The Hermes gzip breach is the informative one. My earlier reasoning was that only
the raw dimension could realistically breach, because a kilobyte of new source
compresses to a few hundred bytes. That held for the SDK, where gzip had 2808
bytes of slack, but it is wrong here: Hermes gzip was cut to 133 bytes above
head. These budgets are set that fine on every dimension, so whichever is
tightest breaches first and patching one at a time invites another CI round for
the same kilobyte of code.

So both breached dimensions are raised to the next 500-byte step above the
observed value, and the sibling dimensions on the same bundles get the same small
allowance -- vite and esbuild gzip/brotli on the client, hermes brotli and the
whole metro triple on mobile. Every increase is 500 to 1500 bytes, proportional
to the roughly one kilobyte of source multiplyPoint adds, and none of them
loosens a budget beyond what that growth accounts for.

I tried to measure these locally rather than infer them, instrumenting
check-wallet-toolbox-platform.mjs to print every dimension the way I did for the
SDK checker. The wallet lanes pack a tarball and resolve it as an external
consumer, which needs CI's setup, so the run fails before measuring. The script
is restored unmodified -- confirmed by an empty diff under scripts/.

Verified: oxlint clean repo-wide, sdk tsc -b clean, the multiplyPoint suite green
at 10 tests, prettier clean on both budget files, the SDK exact-tarball browser
contract still passing at raw 555438 / gzip 159192 / brotli 131956, and the diff
containing nothing but the two budget files.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ty-everett ty-everett changed the title feat(sdk): add optional multiplyPoint for wallet-side point masking feat(wallet): add BRC-98 ECPM semantic module Aug 25, 2026
@ty-everett

Copy link
Copy Markdown
Collaborator

Revised this existing PR/branch in place; no replacement PR was opened.

  • a1eaaa6c0 replaces the optional SDK multiplyPoint method with the
    installable BRC-98 p ecpm semantic module and the backwards-compatible
    Wallet Toolbox handleRequest hook.
  • 4632f51f9 substantively re-verifies the four service/reference pages that
    expired during this run; docs policy, render, and link validation now pass.
  • The complete BRC-229 replacement is posted as an apply-ready review
    suggestion: BRC-229: Wallet-Native Elliptic Curve Point Multiplication as a BRC-98 Module BRCs#230 (comment)

Local verification is recorded in the updated PR description, including the
full workspace suite, 98.52% ECPM statement coverage, 86.44% mutation score,
browser budgets, and exact packed consumers.

The remaining repository-health failure is base governance state, not a feature
failure: three security/quality exceptions reached their owner-review dates on
2026-08-23/24. I have not renewed those exceptions without the required
maintainer governance review.

@raspi-user

Copy link
Copy Markdown

I reviewed the current head 67ec3fd7602a77a3a389399c13fa6484f41d17ad. The direction is much better than adding a new wallet method: keeping ECPM behind a BRC-98 semantic module preserves the BRC-100 surface and makes the wallet host the trusted boundary. The point validation, fixed getPublicKey result shape, and tests for apply/remove round trips are good.

I would not treat this as merge-ready yet, for two reasons:

  1. Privileged grant cache does not include privilegedReason

    In packages/wallet/ecpm-permission-module/src/EcpmPermissionModule.ts, ensureAuthorized() builds an authorization request that includes privilegedReason, but authorizationScope() only caches by originator, security level, logical protocol, counterparty, and privileged/primary status. That means once one privileged ECPM request is approved, a later request with the same protocol scope but a different privilegedReason can skip authorize() and still call privilegedKeyDeriver(parsed.privilegedReason!). For privileged key access, I think the approved reason needs to be part of the grant scope, or the module needs to cache and reuse only the exact approved privileged derivation context.

  2. Concurrent authorization deduping inherits the same broad scope

    The same scope is also used by pendingGrants. Two concurrent privileged requests with different reasons but the same protocol/counterparty can share one approval result. That is probably not what a user prompt implies. If the authorization prompt displays the requested reason, operation, key ID, or point, then the pending/cache key should include the fields whose values the user is actually approving.

  3. PR is still conflicting and GitHub CI is not merge-green

    GitHub currently reports the PR as CONFLICTING, with repository health / early policy / merge-gate failures. The author says the remaining failure is governance state rather than feature code, but from a reviewer perspective this still needs resolving or an explicit maintainer exception before merge.

I would ask for the privileged authorization scope to be tightened, with tests covering changed privilegedReason and concurrent changed privilegedReason requests. I would also want the conflict/CI status resolved before final approval.

Tests reviewed: GitHub checks, PR body, package implementation, Wallet Toolbox hook, and ECPM unit/property tests. I did not run the full workspace locally.

@ty-everett

Copy link
Copy Markdown
Collaborator

Thanks @raspi-user for the review. The privileged-approval scope now includes the exact privilegedReason, and the implementation is aligned with the final BRC-229 character limits, including accepted 353/354-byte outer envelopes and rejection above the bound.

Exact head 7751b42a4 is fully green: 35 successful checks, 3 intentional scope-based skips, no failures or pending checks, and the merge gate passed. All review conversations are resolved and this PR is ready for final maintainer review.

@BraydenLangley BraydenLangley left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran E2E integration tests locally, and it looks good to merge!

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants